-
-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Zip path traversal vulnerability #2630
Conversation
@damagatchi retest this please |
4 similar comments
@damagatchi retest this please |
@damagatchi retest this please |
@damagatchi retest this please |
@damagatchi retest this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good conceptually. Left some very minor comments with only the user message being the blocking one.
@@ -285,6 +285,7 @@ mult.install.bad=The selected ZIP file is not valid. Please choose a valid zip f | |||
mult.install.progress.baddest=Couldn't write multimedia to the local filesystem at: ${0} | |||
mult.install.progress.badentry=There was a bad entry in the zip file: ${0} | |||
mult.install.progress.errormoving=There was a problem copying the multimedia from the zip file, please try again. | |||
mult.install.progress.invalid.entry=The path of the entry ${0} doesn't match the parent folder, review the content of the ZIP file and try again! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably just say Invalid CCZ selected
. User would not necessary understand entry
or zip
here. So lets obscure the details because this issue would probably only surface when someone is deliberately trying to do something malicious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@avazirna this is still pending on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what you mean @shubham1g5 😅. That's on me, I was waiting for the jobs to complete to push the new changes and then I forgot
Logger.log(TAG, "Unzipped entry " + entry.getName()); | ||
|
||
File entryOutput = new File(destination, entry.getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: call it entryFile
and path beloe entryPath
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shubham1g5 At this point, it's unclear whether the zip entry is a directory or a file, so the idea was to use a generic term to apply the verification to both. How strongly do you feel about this? we can always put that logic in a helper method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not strongly, ok to skip.
@damagatchi retest this please |
1 similar comment
@damagatchi retest this please |
@damagatchi retest this please |
@@ -285,7 +285,7 @@ mult.install.bad=The selected ZIP file is not valid. Please choose a valid zip f | |||
mult.install.progress.baddest=Couldn't write multimedia to the local filesystem at: ${0} | |||
mult.install.progress.badentry=There was a bad entry in the zip file: ${0} | |||
mult.install.progress.errormoving=There was a problem copying the multimedia from the zip file, please try again. | |||
mult.install.progress.invalid.entry=The path of the entry ${0} doesn't match the parent folder, review the content of the ZIP file and try again! | |||
mult.install.progress.invalid.ccz=The selected CCZ file is invalid, please review its content and try again! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I like the extra messaging but we probably also don't want to hint that user is expected to be able to inspect content of the ccz - The selected CCZ file is invalid, please verify it and try again!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. I approached this with an attacker's persona in mind but let's keep things simple, updating it.
9f6fb4d
to
8dcda3b
Compare
@damagatchi retest this please |
@damagatchi retest this please |
Summary
This PR is to fix a vulnerability that can be exploited to gain unauthorized access to system or even private folders by using ZIP entries with path traversal characters, more info can be found here. The fix is basically to check whether each Zip entry path matches the destination folder, if it doesn't, then the decompression process is interrupted.
Safety Assurance